FINERACT-2688: Fix loan disbursement for checker-only users via maker-checker permission check by aya-abdallah-FOO · Pull Request #6128 · apache/fineract

aya-abdallah-FOO · 2026-07-13T12:14:35Z

Summary

Fixes a bug where a user holding only a role-specific _CHECKER permission (e.g. DISBURSE_LOAN_CHECKER) was rejected with a permission error instead of being routed to the
pending maker-checker approval queue.

Root causes:

PortfolioCommandSourceWritePlatformServiceImpl.logCommandSource() called validateHasPermissionTo(taskPermission) unconditionally, rejecting checker-only users
CommandSourceService.validateMakerChecker() only marked a command as checked for CHECKER_SUPER_USER, ignoring role-specific _CHECKER permissions

Changes:

logCommandSource(): detect checker-only users (has _CHECKER but not base permission), auto-approve the pending AWAITING_APPROVAL entry; block makers from duplicate pending
submissions
validateMakerChecker(): replace isCheckerSuperUser() with hasNotPermissionForAnyOf("CHECKER_SUPER_USER", permission + "_CHECKER")
Add CommandSourceRepository.findPendingByActionAndEntityAndResource() native SQL query
Add MakerCheckerCheckerOnlyInitiationException and MakerCheckerDuplicatePendingSubmissionException (both → HTTP 403)

JIRA

https://issues.apache.org/jira/browse/FINERACT-2688

Test plan

Enable maker-checker for loan disbursement
Maker submits → queued as AWAITING_APPROVAL
Maker submits again → HTTP 403 error.msg.maker.checker.duplicate.pending.submission
Checker-only user (has DISBURSE_LOAN_CHECKER) submits → auto-approves and disburses
Checker-only with no pending entry → HTTP 403 error.msg.maker.checker.checker.only.cannot.initiate
Existing /makerchecker/{id} approve/reject flow unaffected

Aman-Mittal

Need a integration test for this change before merge

Which explain whole process for this fix for verification

Enable maker-checker for loan disbursement
Maker submits → queued as AWAITING_APPROVAL
Maker submits again → HTTP 403 error.msg.maker.checker.duplicate.pending.submission
Checker-only user (has DISBURSE_LOAN_CHECKER) submits → auto-approves and disburses
Checker-only with no pending entry → HTTP 403 error.msg.maker.checker.checker.only.cannot.initiate
Existing /makerchecker/{id} approve/reject flow unaffected

Aman-Mittal · 2026-07-14T09:11:35Z

-            this.context.authenticatedUser(wrapper).validateHasPermissionTo(wrapper.getTaskPermissionName());
+            final String taskPermission = wrapper.getTaskPermissionName();
+            final boolean hasBasePermission = !currentUser.hasNotPermissionForAnyOf(taskPermission);
+            final boolean hasCheckerPermission = !currentUser.hasNotPermissionForAnyOf("CHECKER_SUPER_USER", taskPermission + "_CHECKER");


This Logic can be simplified more

Aman-Mittal · 2026-07-14T09:16:28Z

@aya-abdallah-FOO also please amend your commit to exclude AI co-author

Aman-Mittal · 2026-07-14T09:44:32Z

@aya-abdallah-FOO Also please sign your commits

aya-abdallah-FOO · 2026-07-14T10:23:59Z

@aya-abdallah-FOO Also please sign your commits

@Aman-Mittal can you please check if all is good?

Aman-Mittal · 2026-07-14T10:34:58Z

Fineract Signed Commits Check / Verify Commit Signatures (pull_request)

yes now properly signed

aya-abdallah-FOO · 2026-07-15T08:54:18Z

@Aman-Mittal can you please run the CI and if you know why would any of them still fail?

aya-abdallah-FOO · 2026-07-15T11:28:57Z

CI Results — Shard 6 passed; Shard 4 and Kafka failures explained below

Shard 6 (our test shard) - Passed on MySQL, MariaDB, PostgreSQL

LoanDisbursementMakerCheckerIntegrationTest lands in Shard 6 of 15 (confirmed via the round-robin sort order). All three DB variants passed on the latest commit (b1fc327).

Shard 4 failures (MySQL, MariaDB, PostgreSQL) — test-suite reshuffling, not a logic bug

Adding LoanDisbursementMakerCheckerIntegrationTest inserts a new class alphabetically between LoanDisbursalDateValidationTest and LoanDisbursementDetailsIntegrationTest, shifting the round-robin shard assignment of every test class that sorts after it. As a result, tests that were previously in Shard 3 on develop (e.g. LoanMultipleDisbursementRepaymentScheduleTest, UndoRepaymentWithDownPaymentIntegrationTest) now run in Shard 4 after the original Shard-4 tests, creating a new execution ordering.

Our production code changes are guarded behind the global MAKER_CHECKER configuration (defaults to false/disabled), so the new logCommandSource paths and CommandSourceRepository query are never exercised in any of the Shard 4 tests. The failures are not in our code's execution path.

One specific pre-existing fragility we found: UndoRepaymentWithDownPaymentIntegrationTest enables ENABLE_BUSINESS_DATE at the top of its @test method without a try/finally — if the test fails mid-way, the config leaks into subsequent tests. This issue exists on develop too but didn't surface before because the test ran in Shard 3 with different predecessors. Could a maintainer check the artifact test-results-*-shard-4-attempt-2 to confirm which specific test is the first to fail? That would let us pinpoint the root cause and file a targeted fix for those tests.

Kafka Smoke Test — transient Docker infrastructure failure

The failure is in step 8 "Build the image" (jibDockerBuild), before any Kafka tests run. This is a Docker daemon / image-pull issue in the CI runner, unrelated to our Java changes (Shard 6 compilation and startup succeeded). Could a maintainer re-run just the Kafka smoke test job? @Aman-Mittal

adamsaghy · 2026-07-16T10:02:34Z

@aya-abdallah-FOO Please review the failing tests.

aya-abdallah-FOO · 2026-07-17T06:36:04Z

CI Results — Shard 6 passed; Shard 4 and Kafka failures explained below

Shard 6 (our test shard) - Passed on MySQL, MariaDB, PostgreSQL

LoanDisbursementMakerCheckerIntegrationTest lands in Shard 6 of 15 (confirmed via the round-robin sort order). All three DB variants passed on the latest commit (b1fc327).

Shard 4 failures (MySQL, MariaDB, PostgreSQL) — test-suite reshuffling, not a logic bug

Adding LoanDisbursementMakerCheckerIntegrationTest inserts a new class alphabetically between LoanDisbursalDateValidationTest and LoanDisbursementDetailsIntegrationTest, shifting the round-robin shard assignment of every test class that sorts after it. As a result, tests that were previously in Shard 3 on develop (e.g. LoanMultipleDisbursementRepaymentScheduleTest, UndoRepaymentWithDownPaymentIntegrationTest) now run in Shard 4 after the original Shard-4 tests, creating a new execution ordering.

Our production code changes are guarded behind the global MAKER_CHECKER configuration (defaults to false/disabled), so the new logCommandSource paths and CommandSourceRepository query are never exercised in any of the Shard 4 tests. The failures are not in our code's execution path.

One specific pre-existing fragility we found: UndoRepaymentWithDownPaymentIntegrationTest enables ENABLE_BUSINESS_DATE at the top of its @test method without a try/finally — if the test fails mid-way, the config leaks into subsequent tests. This issue exists on develop too but didn't surface before because the test ran in Shard 3 with different predecessors. Could a maintainer check the artifact test-results-*-shard-4-attempt-2 to confirm which specific test is the first to fail? That would let us pinpoint the root cause and file a targeted fix for those tests.

Kafka Smoke Test — transient Docker infrastructure failure

The failure is in step 8 "Build the image" (jibDockerBuild), before any Kafka tests run. This is a Docker daemon / image-pull issue in the CI runner, unrelated to our Java changes (Shard 6 compilation and startup succeeded). Could a maintainer re-run just the Kafka smoke test job? @Aman-Mittal

@adamsaghy FYI

IOhacker

LGTM

AshharAhmadKhan · 2026-07-22T19:32:52Z

@aya-abdallah-FOO please resolve conflicts and rebase to latest develop.

…-checker permission check When maker-checker is enabled, a user holding only the <ACTION>_CHECKER permission (e.g. DISBURSE_LOAN_CHECKER) was rejected with a permission error instead of being routed to the pending approval queue. - logCommandSource(): detect checker-only users, look up the pending AWAITING_APPROVAL command for the same action/entity/resource, and auto-approve it; block makers from duplicate pending submissions - validateMakerChecker(): broaden approval condition from isCheckerSuperUser() to also accept role-specific _CHECKER permissions - Add CommandSourceRepository.findPendingByActionAndEntityAndResource() native query (searches all resource-id columns) - Add MakerCheckerCheckerOnlyInitiationException and MakerCheckerDuplicatePendingSubmissionException Signed-off-by: aya.abdallah <aya.abdallah@foo.mobi>

…ix MakercheckerTest The previous change replaced user.isCheckerSuperUser() with a broader userHasCheckerPermission check that also matched task-specific _CHECKER permissions. This caused makers who hold both base and _CHECKER permissions to have their commands auto-approved instead of routed to AWAITING_APPROVAL, breaking MakercheckerTest across all three databases. The checker-only flow introduced by FINERACT-2688 is handled upstream in logCommandSource, so validateMakerChecker never sees checker-only users and needs no change. Signed-off-by: aya.abdallah <aya.abdallah@foo.mobi>

…t maker-checker flow Covers all five scenarios: - Maker submits disbursement → queued as AWAITING_APPROVAL - Maker submits again → HTTP 403 duplicate pending submission - Checker-only user with no pending entry → HTTP 403 cannot initiate - Checker-only user auto-approves pending entry and disburses - Traditional /makerchecker/{id}?command=approve flow unaffected Signed-off-by: aya.abdallah <aya.abdallah@foo.mobi>

Two fixes for the flaky CI failures in MySQL/MariaDB shards: 1. Move global config updates (MAKER_CHECKER, ENABLE_SAME_MAKER_CHECKER) inside the try block so the finally cleanup is guaranteed even if the second update throws — preventing MAKER_CHECKER=true from leaking to subsequent tests in the same shard and causing unexpected maker-checker queuing. 2. Narrow findPendingByActionAndEntityAndResource to match only resource_id, loan_id, and savings_account_id. The previous query also matched client_id, office_id, group_id, product_id, and other contextual columns. When a loan ID coincidentally equalled a contextual ID stored on another pending entry, the wrong command was returned — causing Scenario 3 (checker-only no-entry → 403) to auto-approve the wrong loan instead of throwing, breaking the assertion.

adamsaghy · 2026-07-23T17:09:38Z

+            where upper(c.action_name) = upper(?1)
+            and upper(c.entity_name) = upper(?2)
+            and (
+                c.resource_id = ?3


why ordinal? i think param name like "entityName" would make more sense for readability.

@query

LoanNextPaymentDueAmountIntegrationTest enabled three business events (LoanApprovedBusinessEvent, LoanBalanceChangedBusinessEvent, LoanDisbursalBusinessEvent) in each test method but never disabled them. Adding LoanDisbursementMakerCheckerIntegrationTest alphabetically before it caused it to shift shards, exposing the leaked event state to subsequent tests and causing intermittent CI failures. Add @AfterEach cleanup to reliably disable the events after each method. Also convert the native SQL @query to JPQL so it works identically on MariaDB, MySQL, and PostgreSQL without dialect-specific column names.

…ection Replace the native SQL query (SELECT DISTINCT c.* with nativeQuery=true) with a plain JPQL scalar query that returns only the command source IDs. The native SQL had two failure modes: 1. MySQL/MariaDB: SELECT DISTINCT c.* triggers internal key creation for the TEXT column command_as_json, which MySQL cannot index without an explicit prefix length — resulting in a runtime SQL error. 2. The previous JPQL used DISTINCT on the entity type, causing Hibernate to JOIN-fetch eager @manytoone relationships and then apply DISTINCT over those columns, hitting the same TEXT-column restriction on MariaDB. Returning only c.id avoids both issues: no TEXT columns, no JOIN, no DISTINCT. The callers only needed the ID anyway (they reload the full entity via findById in approveEntry/validateMakerCheckerTransaction).

aya-abdallah-FOO force-pushed the port/CBS-509-loan-disbursement-checker-permission branch 2 times, most recently from 30e1794 to f830f34 Compare July 13, 2026 15:01

Aman-Mittal reviewed Jul 14, 2026

View reviewed changes

Aman-Mittal added bug Needs Functional Review PRs which pass build and have no obvious technical problems, but need functional review. java Pull requests that update Java code labels Jul 14, 2026

aya-abdallah-FOO force-pushed the port/CBS-509-loan-disbursement-checker-permission branch from b71128f to 62ec993 Compare July 14, 2026 09:34

aya-abdallah-FOO force-pushed the port/CBS-509-loan-disbursement-checker-permission branch 2 times, most recently from b0bd55d to 89d50b9 Compare July 14, 2026 10:21

aya-abdallah-FOO force-pushed the port/CBS-509-loan-disbursement-checker-permission branch from 89d50b9 to 106bac4 Compare July 14, 2026 10:59

IOhacker previously approved these changes Jul 18, 2026

View reviewed changes

aya-abdallah-FOO added 4 commits July 23, 2026 09:23

aya-abdallah-FOO force-pushed the port/CBS-509-loan-disbursement-checker-permission branch from d7fc924 to cd65b81 Compare July 23, 2026 06:27

adamsaghy reviewed Jul 23, 2026

View reviewed changes

FINERACT-2688: Use named parameters in native @query for readability

a473151

aya-abdallah-FOO dismissed IOhacker’s stale review via a473151 July 24, 2026 15:05

aya-abdallah-FOO added 2 commits July 27, 2026 10:43

FINERACT-2688: Revert JPQL to native SQL to fix shard 9 test failures

a2bec2b

aya-abdallah-FOO added 2 commits July 28, 2026 10:50

FINERACT-2688: Fix spotless formatting in CommandSourceRepository

b7e7783

Uh oh!

Conversation

aya-abdallah-FOO commented Jul 13, 2026

Summary

JIRA

Test plan

Uh oh!

Aman-Mittal left a comment

Choose a reason for hiding this comment

Uh oh!

Aman-Mittal Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Aman-Mittal commented Jul 14, 2026

Uh oh!

Aman-Mittal commented Jul 14, 2026

Uh oh!

aya-abdallah-FOO commented Jul 14, 2026

Uh oh!

Aman-Mittal commented Jul 14, 2026

Uh oh!

aya-abdallah-FOO commented Jul 15, 2026

Uh oh!

aya-abdallah-FOO commented Jul 15, 2026

Uh oh!

adamsaghy commented Jul 16, 2026

Uh oh!

aya-abdallah-FOO commented Jul 17, 2026

Uh oh!

IOhacker left a comment

Choose a reason for hiding this comment

Uh oh!

AshharAhmadKhan commented Jul 22, 2026

Uh oh!

adamsaghy Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

aya-abdallah-FOO Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants